-
-
Notifications
You must be signed in to change notification settings - Fork 11
Include process names in message before installing package. #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
mast-eu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just one comment, but nothing relevant. For me, this is ok.
Would render #52 obsolete.
| "Plugin Manager", | ||
| "Plugin Manager is going to write to files that are holded by other executables. " + Environment.NewLine + | ||
| "Do you want to kill all instances of these applications?" | ||
| $"Plugin Manager will be writing to files that are currently in use.\r\n\r\nDo you want to stop {processNames}?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| $"Plugin Manager will be writing to files that are currently in use.\r\n\r\nDo you want to stop {processNames}?" | |
| $"Plugin Manager will be writing to files that are currently in use.\r\n\r\nDo you want to stop all instances of {processNames}?" | |
This is really just a detail and I have no strong feelings about it. At the end I accept both forms, with and without this suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no option. I'm not good at user texts, as I tend to put there too much information.
Think it's @RussKie's call 😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose if there's only a single instance of an app, than it is the first message. Else the second ;))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem 😎
RussKie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also suggest changing the messagebox's icon to either question or warning.
LGTM
|
Message now contains or don't information about instances based on process count to kill. |
Fixes #51, an alternative to #52.